Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(clipshot): fix the command on Windows #14

Merged
merged 1 commit into from
Apr 20, 2024
Merged

fix(clipshot): fix the command on Windows #14

merged 1 commit into from
Apr 20, 2024

Conversation

WPMGPRoSToTeMa
Copy link
Contributor

@WPMGPRoSToTeMa WPMGPRoSToTeMa commented Apr 17, 2024

Fixes #13.

%q over-escapes the string and makes a mess. It's enough to just escape the potential ' characters in the file path.

P.S. Many thanks for the clipshot script :)

@ObserverOfTime
Copy link
Owner

Does this work for you? @Justtaway

@Justtaway
Copy link

Justtaway commented Apr 18, 2024

Does this work for you? @Justtaway

No, still doesn't work
Logs are still the same:
image

@ObserverOfTime
Copy link
Owner

ObserverOfTime commented Apr 18, 2024

Thought so. I assume your system tries to run powershell within powershell, which doesn't work.
Ergo, the command needs to be wrapped in cmd and then call powershell with proper quoting.
However, %q messes it up so we would have to manually quote it with escaped " and hope that works.

@ObserverOfTime
Copy link
Owner

Though, I'd like you to try one more thing: replace the , in line 21 with .. and see if that works.

@WPMGPRoSToTeMa
Copy link
Contributor Author

@Justtaway could you please try to run the following in powershell?

powershell -NoProfile -Command 'Add-Type -Assembly System.Windows.Forms, System.Drawing;' '[Windows.Forms.Clipboard]::SetImage([Drawing.Image]::FromFile(''C:\Users\cuzzo\AppData\Local\Temp\mpv-screenshot.jpeg''))'

@Justtaway
Copy link

@Justtaway could you please try to run the following in powershell?

Actually, I think I found the solution. When I typed this command in powershell, I got error that powershell is not a part of cmdlet. Added %SYSTEMROOT%\System32\WindowsPowerShell\v1.0\ to PATH variable and now script is working just fine.

@WPMGPRoSToTeMa
Copy link
Contributor Author

WPMGPRoSToTeMa commented Apr 18, 2024

@ObserverOfTime I opened this PR because the current version does not work for me:

mpv-scripts/clipshot.lua

Lines 20 to 24 in 342c256

'cmd', '/c', string.format(
'powershell -NoProfile -Command %q',
"Add-Type -Assembly System.Windows.Forms, System.Drawing; "..
"[Windows.Forms.Clipboard]::SetImage([Drawing.Image]::FromFile('"..file.."'))"
)

The previous version should work (though I haven't tested it):

mpv-scripts/clipshot.lua

Lines 20 to 25 in 4853d93

'powershell', '-NoProfile', '-Command', ([[& {
Add-Type -Assembly System.Windows.Forms;
Add-Type -Assembly System.Drawing;
$shot = [Drawing.Image]::FromFile(%q);
[Windows.Forms.Clipboard]::SetImage($shot);
}]]):format(file)

But the previous version does not escape $ in the file path. The proposed version (in this PR) works perfectly.

@ObserverOfTime
Copy link
Owner

But the previous version does not escape $ in the file path.

Neither does your version. 🤔

@WPMGPRoSToTeMa
Copy link
Contributor Author

WPMGPRoSToTeMa commented Apr 18, 2024

But the previous version does not escape $ in the file path.

Neither does your version. 🤔

It does not need to be escaped in single quotes in PowerShell (only ' needs to be escaped). But it should be escaped in double quotes (which are produced by %q in the previous version). Additionaly you also need to escape any backticks ` in double quotes.

@WPMGPRoSToTeMa
Copy link
Contributor Author

Added %SYSTEMROOT%\System32\WindowsPowerShell\v1.0\ to PATH variable

@Justtaway do you have any idea why it was missing from PATH? What is your Windows edition? You can check the edition by using winver in the Run dialog (Win + R shortcut).

@Justtaway
Copy link

Added %SYSTEMROOT%\System32\WindowsPowerShell\v1.0\ to PATH variable

@Justtaway do you have any idea why it was missing from PATH? What is your Windows edition? You can check the edition by using winver in the Run dialog (Win + R shortcut).

Honestly, have no Idea why, I tinker a lot with stuff all the time (recently, Microsoft Store stopped updating apps for me, and I probably deleted it and can't reinstall)... Maybe because I installed new version of powershell (7.4). I have Win11 Pro 23H2 22631.3374.

@WPMGPRoSToTeMa
Copy link
Contributor Author

Maybe because I installed new version of powershell (7.4).

Does this also work for you?

pwsh -NoProfile -Command 'Add-Type -Assembly System.Windows.Forms, System.Drawing;' '[Windows.Forms.Clipboard]::SetImage([Drawing.Image]::FromFile(''C:\Users\cuzzo\AppData\Local\Temp\mpv-screenshot.jpeg''))'

@Justtaway
Copy link

Does this also work for you?

image

@ObserverOfTime
Copy link
Owner

ObserverOfTime commented Apr 18, 2024

Seems like it would work but the file doesn't exist anymore?

@WPMGPRoSToTeMa
Copy link
Contributor Author

My thoughts:
The scenario when powershell.exe is not on PATH is not normal and should not be supported by the script. It's more like it was removed from PATH manually or by some weird script. If anyone else will have the same issue they should verify that powershell.exe is on PATH. However I think it might be desirable to call pwsh.exe if it's available and it actually works fine (with the priority over powershell.exe), but may be it's not needed right now (I also have concerns about old installations with 6.x versions which may not work correctly with WinForms assembly).

@ObserverOfTime ObserverOfTime merged commit 881b043 into ObserverOfTime:master Apr 20, 2024
3 checks passed
@ObserverOfTime
Copy link
Owner

Thank you.

@WPMGPRoSToTeMa WPMGPRoSToTeMa deleted the patch-1 branch April 20, 2024 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Screenshot created but not copied to clipboard
3 participants